Propose a Scheduled condition for RB/CRB #823

dddddai · 2021-10-16T06:01:50Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Please refer to #821 (comment)

Which issue(s) this PR fixes:
Fixes #821

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

RainbowMango · 2021-10-16T08:41:40Z

pkg/scheduler/scheduler.go

+// setScheduledCondition sets the Scheduled condition for the given binding
+func setScheduledCondition(status *workv1alpha2.ResourceBindingStatus, scheduled metav1.ConditionStatus, scheduleType ScheduleType) {
+	newScheduledCondition := metav1.Condition{
+		Type:               workv1alpha2.Scheduled,
+		Status:             scheduled,
+		Reason:             string(scheduleType),
+		LastTransitionTime: metav1.Now(),
+	}
+
+	meta.SetStatusCondition(&status.Conditions, newScheduledCondition)
+}


Why would we distinguish the schedule type here?
The lastTransitionTime is the last time the condition transitioned from <none> status to 'Scheduled'.
The condition should not change from Scheduled(first schedule) to Scheduled(reconcile schedule).

@RainbowMango Thanks for your review!

Why would we distinguish the schedule type here?

I just tried to figure out a good condition reason and used schedule type as the reason

The condition should not change from Scheduled(first schedule) to Scheduled(reconcile schedule).

Why? Do you mean the reason should be a constant string? Sorry I didn't get the point, please correct me if I'm wrong

I think The Scheduled condition represents if an RB has been scheduled or not, actually we don't care the reason(at least for now). And the lastTransitionTime is basically the scheduled time.

RainbowMango

So quick!
Appreciate!

RainbowMango · 2021-10-16T12:12:40Z

pkg/scheduler/scheduler.go

+	setScheduledCondition(&binding.Status, metav1.ConditionTrue, scheduleSuccessReason, scheduleSuccessMessage)
+	_, err = s.KarmadaClient.WorkV1alpha2().ResourceBindings(binding.Namespace).UpdateStatus(context.TODO(), binding, metav1.UpdateOptions{})


Just a question, if the condition already exists, can we ignore the update here?

Oh yes, sorry for my oversight, fixed

RainbowMango · 2021-10-16T12:12:56Z

/assign @XiShanYongYe-Chang

RainbowMango

Glad to see that you added the E2E test.
/approve
leave LGTM to @XiShanYongYe-Chang for the E2E test part.

XiShanYongYe-Chang

It's generally fine, some suggestions can be considered.

pkg/scheduler/scheduler.go

XiShanYongYe-Chang · 2021-10-18T02:59:24Z

test/e2e/failover_test.go

@@ -152,6 +152,15 @@ var _ = ginkgo.Describe("failover testing", func() {
 				fmt.Printf("reschedule in %d target cluster\n", totalNum)
 			})

+			ginkgo.By("check if the scheduled condition is true", func() {


I think it is unnecessary to add this judgment to determine the conition of binding.

If the condition Scheduled is not set to true, the resource will not be propagated to member clusters and the previous test will fail. Same as bellow.

How do you think?

@XiShanYongYe-Chang Thanks for your review!
Yes it seems unnecessary now, but it can make sure the scheduled condition is True if any changes are made in future, in which case this test should work.

From this point of view, the test is reasonable, so I'm inclined to keep it.

Signed-off-by: dddddai <[email protected]>

XiShanYongYe-Chang

Reserve E2E, some nits, other lgtm.

XiShanYongYe-Chang · 2021-10-18T06:05:29Z

test/e2e/failover_test.go

+				rb, err := getResourceBinding(deployment)
+				gomega.Expect(err).ShouldNot(gomega.HaveOccurred())


Need to put this logic into PollImmediate function, otherwise will judge the same rb until timeout.

Same as other parts.

XiShanYongYe-Chang · 2021-10-18T06:07:23Z

test/e2e/scheduling_test.go

+
+// get the resource binding associated with the workload
+func getResourceBinding(workload interface{}) (*workv1alpha2.ResourceBinding, error) {
+	uncastObj, _ := runtime.DefaultUnstructuredConverter.ToUnstructured(workload)


Add err judgement:

uncastObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(workload) gomega.Expect(err).ShouldNot(gomega.HaveOccurred())

Thanks for reminding, fixed, PTAL:)

Signed-off-by: dddddai <[email protected]>

XiShanYongYe-Chang

Thank you.

/lgtm

karmada-bot · 2021-10-18T07:14:34Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: RainbowMango

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 16, 2021

karmada-bot requested review from Garrybest, mrlihanbo and XiShanYongYe-Chang October 16, 2021 06:01

karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 16, 2021

RainbowMango reviewed Oct 16, 2021

View reviewed changes

dddddai force-pushed the sched-condition branch 5 times, most recently from d4076f4 to 37b3b67 Compare October 16, 2021 11:54

RainbowMango reviewed Oct 16, 2021

View reviewed changes

karmada-bot assigned XiShanYongYe-Chang Oct 16, 2021

dddddai force-pushed the sched-condition branch from 37b3b67 to edd1fd2 Compare October 16, 2021 12:39

RainbowMango approved these changes Oct 18, 2021

View reviewed changes

XiShanYongYe-Chang reviewed Oct 18, 2021

View reviewed changes

add scheduled condition for binding

eaea5c6

Signed-off-by: dddddai <[email protected]>

dddddai force-pushed the sched-condition branch from edd1fd2 to 05aa023 Compare October 18, 2021 03:44

XiShanYongYe-Chang reviewed Oct 18, 2021

View reviewed changes

dddddai force-pushed the sched-condition branch from 05aa023 to 57ad4a0 Compare October 18, 2021 06:30

add e2e test for scheduled condition

57ad4a0

Signed-off-by: dddddai <[email protected]>

XiShanYongYe-Chang reviewed Oct 18, 2021

View reviewed changes

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2021

RainbowMango added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2021

karmada-bot merged commit d2af1e1 into karmada-io:master Oct 18, 2021

XiShanYongYe-Chang mentioned this pull request Oct 19, 2021

adapt the Scheduled condition for binding #838

Merged

dddddai deleted the sched-condition branch October 24, 2021 02:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propose a Scheduled condition for RB/CRB #823

Propose a Scheduled condition for RB/CRB #823

dddddai commented Oct 16, 2021

RainbowMango Oct 16, 2021

dddddai Oct 16, 2021

RainbowMango Oct 16, 2021

RainbowMango left a comment

RainbowMango Oct 16, 2021

dddddai Oct 16, 2021

RainbowMango commented Oct 16, 2021

RainbowMango left a comment

XiShanYongYe-Chang left a comment

XiShanYongYe-Chang Oct 18, 2021

dddddai Oct 18, 2021 •

edited

Loading

XiShanYongYe-Chang Oct 18, 2021

XiShanYongYe-Chang left a comment

XiShanYongYe-Chang Oct 18, 2021

XiShanYongYe-Chang Oct 18, 2021

dddddai Oct 18, 2021

XiShanYongYe-Chang left a comment

karmada-bot commented Oct 18, 2021

		setScheduledCondition(&binding.Status, metav1.ConditionTrue, scheduleSuccessReason, scheduleSuccessMessage)
		_, err = s.KarmadaClient.WorkV1alpha2().ResourceBindings(binding.Namespace).UpdateStatus(context.TODO(), binding, metav1.UpdateOptions{})

		rb, err := getResourceBinding(deployment)
		gomega.Expect(err).ShouldNot(gomega.HaveOccurred())

Propose a Scheduled condition for RB/CRB #823

Propose a Scheduled condition for RB/CRB #823

Conversation

dddddai commented Oct 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RainbowMango left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RainbowMango commented Oct 16, 2021

RainbowMango left a comment

Choose a reason for hiding this comment

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dddddai Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

karmada-bot commented Oct 18, 2021

dddddai Oct 18, 2021 •

edited

Loading